[compiler] Allow inferred non-optional paths when manual deps were optional #30816
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tional If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. [ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This was referenced Aug 26, 2024
josephsavona
added a commit
that referenced
this pull request
Aug 26, 2024
…tional If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. ghstack-source-id: 8f34860deafefe48c7a374caa1b55482e571308d Pull Request resolved: #30816
facebook-github-bot
added
CLA Signed
React Core Team
…eps were optional" If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. [ghstack-poisoned]
…eps were optional" If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. [ghstack-poisoned]
…eps were optional" If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. [ghstack-poisoned]
mofeiZ
approved these changes
Aug 28, 2024
There was a problem hiding this comment.
The change makes sense intuitively, but I'm not sure if it precisely preserves previous memo behavior.
Take the current test fixture:
function Component(props) {
const data = useMemo(() => {
// actual code is non-optional
return props.items.edges.nodes ?? [];
// deps are optional
}, [props.items?.edges?.nodes]);
return <Foo data={data} />;
}Let's say the initial render used props = {items: {edges: {nodes: undefined}}}. The useMemo would take a dep on [undefined], and we get data = [].
If the next render passes props = {items: undefined}, the useMemo dependencies don't change (and we would reuse data = [[ previous_result ]]). With Forget enabled, this would throw.
Just wanted to note that this could technically happen. If we want to preserve precise semantics of existing memo, we might need to use a strict optional comparison.
|
Good catch, i'll update this to ensure that the inferred deps exactly match the optionality of the manual deps (will fix-forward though) |
josephsavona
added a commit
that referenced
this pull request
Aug 28, 2024
…tional If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. ghstack-source-id: 9f7a99ee5f7caa2c2d96f70f360e4320bac3de2d Pull Request resolved: #30816
This was referenced Aug 29, 2024
This was referenced Sep 6, 2024
gnoff
pushed a commit
to vercel/next.js
that referenced
this pull request
Sep 12, 2024
**breaking change for canary users: Bumps peer dependency of React from `19.0.0-rc-7771d3a7-20240827` to `19.0.0-rc-94e652d5-20240912`** [diff facebook/react@7771d3a7...94e652d5](facebook/react@7771d3a...94e652d) <details> <summary>React upstream changes</summary> - facebook/react#30952 - facebook/react#30950 - facebook/react#30946 - facebook/react#30934 - facebook/react#30947 - facebook/react#30945 - facebook/react#30938 - facebook/react#30936 - facebook/react#30879 - facebook/react#30888 - facebook/react#30931 - facebook/react#30930 - facebook/react#30832 - facebook/react#30929 - facebook/react#30926 - facebook/react#30925 - facebook/react#30905 - facebook/react#30900 - facebook/react#30910 - facebook/react#30906 - facebook/react#30899 - facebook/react#30919 - facebook/react#30708 - facebook/react#30907 - facebook/react#30897 - facebook/react#30896 - facebook/react#30895 - facebook/react#30887 - facebook/react#30889 - facebook/react#30893 - facebook/react#30892 - facebook/react#30891 - facebook/react#30882 - facebook/react#30881 - facebook/react#30870 - facebook/react#30849 - facebook/react#30878 - facebook/react#30865 - facebook/react#30869 - facebook/react#30875 - facebook/react#30800 - facebook/react#30762 - facebook/react#30831 - facebook/react#30866 - facebook/react#30853 - facebook/react#30850 - facebook/react#30847 - facebook/react#30842 - facebook/react#30837 - facebook/react#30848 - facebook/react#30844 - facebook/react#30839 - facebook/react#30802 - facebook/react#30841 - facebook/react#30827 - facebook/react#30826 - facebook/react#30825 - facebook/react#30824 - facebook/react#30840 - facebook/react#30838 - facebook/react#30836 - facebook/react#30819 - facebook/react#30816 - facebook/react#30814 - facebook/react#30813 - facebook/react#30812 - facebook/react#30811 </details> --------- Co-authored-by: vercel-release-bot <infra+release@vercel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation.
The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule.